-
Notifications
You must be signed in to change notification settings - Fork 142
Various composefs work #1662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various composefs work #1662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant amount of work related to composefs, including switching the fs-verity hashing algorithm from SHA256 to SHA512, adding a new workflow for building 'sealed' images with Unified Kernel Images (UKIs), and making the composefs backend a default feature. The changes are mostly consistent and well-structured, with good use of type aliases to improve code clarity.
However, I've found a few issues that need attention:
- There's some dead and potentially buggy code in the new
xtask
for building sealed images. - A new Dockerfile contains a leftover command that should be removed.
- The documentation for newly added CLI options and subcommands is incomplete.
Please see my detailed comments for suggestions on how to address these points.
**--composefs-native** | ||
|
||
|
||
|
||
**--insecure** | ||
|
||
|
||
|
||
Default: false | ||
|
||
**--bootloader**=*BOOTLOADER* | ||
|
||
|
||
|
||
Default: grub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| **bootc usr-overlay** | Add a transient writable overlayfs on `/usr` | | ||
| **bootc install** | Install the running container to a target | | ||
| **bootc container** | Operations which can be executed as part of a container build | | ||
| **bootc composefs-finalize-staged** | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split prep work for this to #1665 |
837dc44
to
cccc2f8
Compare
@jeckersb I ended up reworking this one to use the bootc-in-container for the sealing side because it would be too annoying in our CI to have it be on the host (which is ubuntu here). |
I'm guessing this is something to do with me running
|
Hmm I don't know what's up with that error. Offhand it's perhaps something to do with the source bind path when it's relative? Does it work if you make it an absolute path? What's your wrapper for |
Nope still doesn't work with absolute paths
I don't have any wrapper for
and then...
I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow. Given the eyeball test the changes look good to me. From CI it looks like it needs a |
Testing a stripped-down case this morning, it works fine for me to pass secrets inside of the toolbox over podman-remote to the host. I've also verified that if I run the original podman command directly on the host, it works properly. So I'm thinking now that it has something to do with passing secrets over podman-remote in combination with multistage builds. |
Ahhhhhh it's this - containers/podman#25314 I'll put in a separate PR to add that silly workaround to our .dockerignore |
3dbc6dd
to
ee05675
Compare
OK it took me a bit too long to figure out the reason that we were getting This will get naturally fixed when we move to using bcvk here. However...for now I moved the outer portion of the sealing back to shell script - we've already moved some of the inner portion into the Rust code. |
ee05675
to
a58d0a3
Compare
OK though, unfortunately right now tmt's virtual provisioner doesn't support uefi. That looks pretty easy to fix, but I'm a bit tempted to try out having |
bb56008
to
b70bee9
Compare
fda247e
to
10b2832
Compare
Is that the primary thing blocking this at this point? |
Signed-off-by: Colin Walters <[email protected]>
This ensures we're SHA-512 across the board. Signed-off-by: Colin Walters <[email protected]>
- Use bash strict mode more consistently - Drop the error redirections which can mask problems as recommended by AI Signed-off-by: Colin Walters <[email protected]>
f500118
to
39fffbd
Compare
Yeah I think so, filed teemtee/tmt#4203 That said, we can work around this by using bcvk to provision a system external to tmt. That's not hard, but the downside is that it's logic that'd need to be replicated into anything else that wants to use tmt. |
39fffbd
to
1c4b1d9
Compare
This looks good testing it out on my end, looks like CI is failing because |
1c4b1d9
to
c5c0137
Compare
Yeah I'd messed up the bootupd detection, fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, assuming the in-flight jobs pass just needs trivial validation cleanup
- Change the install logic to detect UKIs and automatically enable composefs - Change the install logic to detect absence of bootupd and default to installing systemd-boot - Move sealing bits to the toplevel - Add Justfile entrypoints - Add basic end-to-end CI coverage (install + run) using our integration tests - Change lints to ignore `/boot/EFI` Signed-off-by: Colin Walters <[email protected]>
c5c0137
to
77685f0
Compare
Holy 🐮 so I removed the background Of course, us having to clean up tons of garbage from the stock runners is ridiculous and hopefully GH will implement #1669 But while let's not kill CI for this one, I'm going to try to go back to optimizing the provisioner... |
OK no, it's just 2/4 jobs here that had some kind of pathological slowness going on, but we don't have enough timing information to have a good idea of what specifically that was. Working on a followup commit. |
No description provided.